Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Look for environment-specific UIO landing URLs first #397

Merged
merged 3 commits into from
Aug 19, 2021

Conversation

kalvinwang
Copy link
Contributor

@kalvinwang kalvinwang commented Aug 19, 2021

This PR looks for a new environment variable to use as the UIO landing URL. If not found, it falls back to the current production/public UIO landing URL.

After this PR is merged, I'll add the SIT07 landing URLs to our test environment and test there

===

  • Relevant documentation (e.g. READMEs, Technical Foundation) updated
  • Issue AC are copied to this PR & are met
  • Changes are tested on Staging post-merge into main

const uioLandingUrl = process.env.URL_UIO_LANDING ?? getUrl('uio-home-url-desktop')
const uioMobileLandingUrl = process.env.URL_UIOMOBILE_LANDING ?? getUrl('uio-home-url-mobile')

const uioHomeLink = userArrivedFromUioMobile ? uioMobileLandingUrl : uioLandingUrl
if (uioHomeLink) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rocketnova I don't understand this if statement-- is it to protect against the possibility of uio-home-url-desktop/mobile, not existing as a string? If they do exist, then uioHomeLink will always be true right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typescript will yell if we don't narrow here, because the code doesn't know those json strings definitely exist - we have a ticket to see if there's some way to assert the json strings do exist #338

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getUrl() can return undefined. I'm not positive what happens if public/urls.json disappears or is malformed or has a read-error, but there's definitely the possibility that public/urls.json exists and the linkKey passed in doesn't exist in the file.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typescript will yell if we don't narrow here, because the code doesn't know those json strings definitely exist - we have a ticket to see if there's some way to assert the json strings do exist #338

Agreed that we should know at build time, but I think the check is important in case something horrible and unexpected happens at runtime

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, this all makes sense. Thanks!

README.md Outdated
@@ -26,6 +26,7 @@ relates to weekly certification
- CERTIFICATE_DIR: The path to the client certificate (certificate must be in PFX/P12 format)
- PFX_FILE: The name of the client certificate file
- (Optional) PFX_PASSPHRASE: The import passphrase for the client certificate if there is one
- (Optional) URL_UIO_LANDING, URL_UIOMOBILE_LANDING: The environment-specific links to the UIO landing page
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- (Optional) URL_UIO_LANDING, URL_UIOMOBILE_LANDING: The environment-specific links to the UIO landing page
- (Optional) URL_UIO_LANDING, URL_UIOMOBILE_LANDING: The environment-specific links to the UIO landing pages

const uioLandingUrl = process.env.URL_UIO_LANDING ?? getUrl('uio-home-url-desktop')
const uioMobileLandingUrl = process.env.URL_UIOMOBILE_LANDING ?? getUrl('uio-home-url-mobile')

const uioHomeLink = userArrivedFromUioMobile ? uioMobileLandingUrl : uioLandingUrl
if (uioHomeLink) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typescript will yell if we don't narrow here, because the code doesn't know those json strings definitely exist - we have a ticket to see if there's some way to assert the json strings do exist #338

Copy link
Contributor

@rocketnova rocketnova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Comment on lines 18 to 22
// Optional environment-specific links back to the UIO landing page, used by EDD testing
const uioLandingUrl = process.env.URL_UIO_LANDING ?? getUrl('uio-home-url-desktop')
const uioMobileLandingUrl = process.env.URL_UIOMOBILE_LANDING ?? getUrl('uio-home-url-mobile')

const uioHomeLink = userArrivedFromUioMobile ? uioMobileLandingUrl : uioLandingUrl
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Not necessary for launch, but I think this is enough logic to put it somewhere else (like in getUrl.ts) rather than in the component.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, I should have put it there. Moved!

const uioLandingUrl = process.env.URL_UIO_LANDING ?? getUrl('uio-home-url-desktop')
const uioMobileLandingUrl = process.env.URL_UIOMOBILE_LANDING ?? getUrl('uio-home-url-mobile')

const uioHomeLink = userArrivedFromUioMobile ? uioMobileLandingUrl : uioLandingUrl
if (uioHomeLink) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getUrl() can return undefined. I'm not positive what happens if public/urls.json disappears or is malformed or has a read-error, but there's definitely the possibility that public/urls.json exists and the linkKey passed in doesn't exist in the file.

@kalvinwang kalvinwang merged commit 2545e0d into main Aug 19, 2021
@kalvinwang kalvinwang deleted the kalvin-landing-urls branch August 19, 2021 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants